Harden code verification: input validation, bounded drift window, and replay protection support (RFC 6238 §5.2) - #12
Conversation
…revent trivially brute-forceable codes
…play protection (RFC 6238 5.2)
These are worthwhile fixes.
Please remove this change. I don't want to dictate how someone can use this library; It's totally fine if someone wants to detect clock drift by burning CPU time.
Please remove this change. This is outside the scope of this library. RFC 6238 Section 5.2 specifies that validation systems must enforce a one-time-only use policy for each generated code: the secret/timeslice combination can't be re-used once it yields a successful authentication. This library does not have a datastore nor does it maintain state, so it cannot enforce this in any meaningful way. The higher level application will have to implement this policy. The changes are otherwise quite long, the diff touches files/content that aren't relevant to the PR's changes. If you want to re-submit this, please tell your AI agent to make surgical changes only and to be concise. |
Summary
This PR hardens
verifyCode()/setCodeLength()against a few subtle but practicalsecurity issues, without changing behavior for any currently-valid usage. All changes
are covered by new unit tests (+94 lines, full suite green).
Problems addressed
1.
verifyCode()hardcodes a 6-digit check, silently breaking 8-digit configurations (bug + security)verifyCode()rejects any code whose length is not exactly 6:But the library supports
setCodeLength(8). In that configurationgetCode()produces8-digit codes while
verifyCode()rejects all of them — an integrator who configuredlonger (stronger) codes gets a verifier that never succeeds, and the natural "fix" on
their side is to roll their own comparison, losing the
hash_equals()timing-safe path.Fix: validate against the configured
$this->_codeLength, and additionally requirectype_digit($code)so non-numeric input is rejected before any HMAC work.2.
setCodeLength()accepts values that produce broken or trivially brute-forceable codesAny int was accepted. Values < 6 produce codes weaker than RFC 4226 permits (§4 R4:
"The value displayed on the token MUST be easily read … at least a six-digit code");
values > 9 overflow the modulo table and previously failed obscurely at generation time.
Fix:
setCodeLength()now throwsInvalidArgumentExceptionoutside the 6–8 range(the range Google Authenticator-compatible apps actually support).
3. Unbounded
$discrepancyallows arbitrarily large validation windowsverifyCode($secret, $code, PHP_INT_MAX)would happily loop over an astronomically largewindow. A caller passing an unvalidated, attacker-influenced discrepancy value would
massively inflate the brute-force acceptance window (each extra slice adds ~2×10⁻⁶
acceptance probability per guess for 6-digit codes) and burn CPU on HMAC computations.
Fix:
$discrepancyis bounded to 0–60 slices (±30 minutes), throwingInvalidArgumentExceptionbeyond that. RFC 6238 §5.2 recommends "at most one time step"of transmission delay allowance; 60 is already a generous ceiling for clock-skew use cases.
4. No way to implement RFC 6238 §5.2 replay protection
RFC 6238 §5.2 requires that a verifier "MUST NOT accept the second attempt of the OTP
after the successful validation". Callers currently can't implement this because
verifyCode()doesn't report which time slice matched — a code remains valid for thewhole discrepancy window, so an attacker who observes a code (shoulder-surfing, phishing
proxy, logs) can replay it.
Fix: an optional by-reference out-parameter:
On success it receives the matched slice; integrators persist the last accepted slice per
user and reject any match with
slice <= lastAccepted. Usage is documented in the README.Backward compatibility
setCodeLength()/verifyCode()now throw on inputs that previously produced silently broken or dangerous behavior — this only surfaces in code that was already misconfigured.Testing
composer test→ OK.